Scotland class 5 - karma clone#227
Conversation
bonboh
left a comment
bonboh
left a comment
There was a problem hiding this comment.
Great job @M-hayek! Nice to see semantic HTML 👍 I've left some comments and tips for you to read, but you don't have to do anything else. Generally, the main things to note are:
- Software teams typically use class names for styling, not ID, so the HTML document doesn't need to have as many IDs
- Code formatting matters to software teams, so try and style it with proper indents and style
- We should use relative units like
rems by default rather thanpxs
|
|
||
| body { | ||
| font-family: 'Roboto', sans-serif; | ||
| font-size: 16px; |
There was a problem hiding this comment.
| font-size: 16px; | |
| font-size: 1rem; |
We should use relative units like rem, especially for font-size. If we use pixels, then if a user changes their browser default font size setting then the screen doesn't increase in size
| width:100%; | ||
| height:100%; |
There was a problem hiding this comment.
| width:100%; | |
| height:100%; |
We don't need these properties so we can remove them
| text-align: center; | ||
| line-height: 25px; | ||
| box-sizing: border-box; /*padding included in the box size*/ | ||
| display: block; /*each element as a block*/ |
There was a problem hiding this comment.
| display: block; /*each element as a block*/ |
Normally, it's not a good idea to turn every element into a block, because when we use inline elements like <a> and <strong> for example, they will all be on separate lines which does not make sense
|
|
||
| #nav-bar {position:fixed; | ||
| top:0; | ||
| height: 70px; |
There was a problem hiding this comment.
| height: 70px; | |
| height: 4.5rem; |
We should use relative units where possible. Same applies to all the other uses of px in this file
| #nav-bar ul a:hover {color:#F15B2A; | ||
| } |
There was a problem hiding this comment.
| #nav-bar ul a:hover {color:#F15B2A; | |
| } | |
| #nav-bar ul a:focus, | |
| #nav-bar ul a:hover { | |
| color:#F15B2A; | |
| } |
We should also add :focus so that keyboard users who use the Tab key to jump to the link also get a change in colour.
Also, for coding the formatting of the code matters in software teams. Typically, it's structured like in my suggestion. This applies to the whole file
| footer ul li img : hover {cursor:pointer; | ||
| } |
There was a problem hiding this comment.
| footer ul li img : hover {cursor:pointer; | |
| } |
The icons aren't clickable so we shouldn't add a mouse pointer on hover. If we want a mouse pointer on hover, we should put the icons in a link <a>
| </nav> | ||
| </header> <!--header ends here--> | ||
|
|
||
| <article> |
There was a problem hiding this comment.
| <article> | |
| <main> |
This seems like it should be a main, since we normally have header main footer as the top level elements on a page
| <header id="header"> | ||
| <nav id="nav-bar"> |
There was a problem hiding this comment.
| <header id="header"> | |
| <nav id="nav-bar"> | |
| <header class="header"> | |
| <nav class="nav-bar"> |
Normally, software teams use classes for styling and not IDs. The same applies for the rest of this file
|
|
||
| <!--content starts here--> | ||
| <section id="content"> | ||
| <h1> Everyone needs a little Karma.</h1> |
There was a problem hiding this comment.
| <h1> Everyone needs a little Karma.</h1> | |
| <h2> Everyone needs a little Karma.</h2> |
We should only have one h1 on the page, so we should update this to h2
|
|
||
|
|
||
| <footer> | ||
| <p> Join us on </p> |
There was a problem hiding this comment.
| <p> Join us on </p> | |
| <h2> Join us on </h2> |
This seems like it could be a heading, so we could make it one
2 participants
Volunteers: Are you marking this coursework? You can find a guide on how to mark this coursework in
HOW_TO_MARK.mdin the root of this repositoryYour Details
Homework Details
Notes
What did you find easy?
What did you find hard?
What do you still not understand?
Any other notes?